-
Notifications
You must be signed in to change notification settings - Fork 74
Allow Medusa to run init function (ex: setUp) once per fuzz run #618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…edusa into add-init-fn-per-fuzz-run
|
@0xZRA appreciate the quick turnaround! I need a few days before I can get to reviewing it just FYI :) |
Absolutely! And thanks for the opportunity ;) |
|
My bad, I never noticed the progression on this PR. This is great thanks @0xZRA. I tested it on a larger codebase and it works In term of usability, I am wondering it would make sense to add a flag to enable |
|
@montyly Thanks for your feedback! I can work on adding the flag, it may take me few days though |
|
@montyly Let me know if the latest commit delivers the intended improvement to devx. My assumption is that the default configuration values will still be processed downstream to address all contracts that implement it. |
|
thanks for the quick work! To be honest I would even go further, and have the flag The reasoning is that all the foundry-based projects uses a |
|
@montyly let me know if the latest approach works.
These are possible scenarios that I can think of: // Same can be achieved by passing // Result: initFunctions = ["setUp", "setUp", "setUp"] // ================================================================ // Result: initFunctions = ["initialize", "initialize"] // ================================================================ // Result: initFunctions = ["setUp", "initialize"] // ================================================================ // Result: initFunctions = ["setUp", "setUp"] // ================================================================ // Result: initFunctions = ["", ""] // ================================================================ // Result: initFunctions = ["", ""] |
@anishnaik are you still planning to look into this at some point? |
|
Hey @0xZRA sorry sorry, medusa has taken a backseat for the past few months in terms of priorities. Will give this a look in the next week. |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Create and send the transaction | ||
| destAddr := contractAddr | ||
| msg := calls.NewCallMessage(fuzzer.deployer, &destAddr, 0, big.NewInt(0), | ||
| fuzzer.config.Fuzzing.BlockGasLimit, nil, nil, nil, callData) | ||
| msg.FillFromTestChainProperties(testChain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace nonexistent BlockGasLimit config field
chainSetupFromCompilations builds the init-call transaction with fuzzer.config.Fuzzing.BlockGasLimit, but FuzzingConfig defines no such field (only TransactionGasLimit). The code will not compile because the struct has no BlockGasLimit member. This should use an existing gas limit value or add the missing field before merging.
Useful? React with 👍 / 👎.
| // Process target contracts init functions | ||
| targetContractsCount := len(fuzzer.config.Fuzzing.TargetContracts) | ||
| initConfigCount := len(fuzzer.config.Fuzzing.TargetContractsInitFunctions) | ||
|
|
||
| // Add initialization functions for target contracts | ||
| for i := 0; i < targetContractsCount; i++ { | ||
| initFunction := "" // Default: no initialization | ||
|
|
||
| if fuzzer.config.Fuzzing.UseInitFunctions { | ||
| if i < initConfigCount && fuzzer.config.Fuzzing.TargetContractsInitFunctions[i] != "" { | ||
| // Use explicit per-contract config | ||
| initFunction = fuzzer.config.Fuzzing.TargetContractsInitFunctions[i] | ||
| } else if len(fuzzer.config.Fuzzing.TargetContractsInitFunctions) == 1 { | ||
| // If only one init function specified (like "setUp") apply it to all contracts | ||
| initFunction = fuzzer.config.Fuzzing.TargetContractsInitFunctions[0] | ||
| } | ||
| } | ||
| initFunctions = append(initFunctions, initFunction) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align init functions to deployment order
The list of init functions is appended in the original TargetContracts order, but contracts are deployed in deploymentOrder when that list exists. When the order of deployments differs from the config array (e.g., because of library dependencies), initFunctions[i] will be applied to the wrong contract. This can cause init calls to target the wrong ABI or revert unnecessarily. Map functions by contract name or build initFunctions in the same order as contractsToDeploy.
Useful? React with 👍 / 👎.
@anishnaik no worries! I'll review |
Closes #611
I decided to follow existing pattern of
TargetContractsandTargetContractsBalancessequential deployments. I think the core functionality is in place (and I hope it meets the requirements?), but the PR could be improved with more edge case testing.@anishnaik @montyly please let me know your thoughts whenever you get a chance